-
Notifications
You must be signed in to change notification settings - Fork 41
ENH: Replace pydicom deprecated usage of get_frame_offsets #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
ENH: Replace pydicom deprecated usage of get_frame_offsets #120
Conversation
|
@jamesobutler thank you so much for the contribution! I asked Copilot about the failing test, and this was the (most relevant, I think, part of the) response:
from typing import Any, Callable, Optional
import pydicom.encaps as _pydicom_encaps # type: ignore
# Resolve functions dynamically (use getattr so mypy does not require the attribute in stubs)
_parse_basic_offsets: Optional[Callable[..., Any]] = getattr(
_pydicom_encaps, 'parse_basic_offsets', None
)
_get_frame_offsets: Optional[Callable[..., Any]] = getattr(
_pydicom_encaps, 'get_frame_offsets', None
)
if _parse_basic_offsets is not None:
parse_basic_offsets = _parse_basic_offsets # type: ignore
_use_parse_basic_offsets = True
elif _get_frame_offsets is not None:
get_frame_offsets = _get_frame_offsets # type: ignore
_use_parse_basic_offsets = False
else:
# Should not happen for a standard pydicom install. Make the error explicit.
raise ImportError(
'Neither parse_basic_offsets nor get_frame_offsets available in pydicom.encaps'
)Or we could just require latest pydicom? |
d01e29c to
9e866aa
Compare
|
The mypy check should no longer fail following dropping of end-of-life Python 3.9. The Python 3.9 CI install of the application is pulling pydicom 2.4.4 because pydicom 3 requires Python 3.10+. I have added testing for Python 3.14 following the drop of Python 3.9. Whenever dicomweb-client last dropped python version support, the justification was because they were end-of-life versions as well (see #113). I would recommend a v0.61.0 release considering the python requirements will have changed. Similarly in dicomweb-client v0.60.0 there was a bump in the python requirement. |
|
I do not think it's a good idea to try and support pydicom across the major version change 2.x to 3.x. I would favour requiring pydicom>=3.0.0 and removing the import hackery |
|
I am also on board with dropping 3.9 support now that it is EOL |
|
@jamesobutler I merged #121, could you please remove those changes from this PR |
|
Yes, I'll rebase this branch upon integration of #122. |
I was thinking the same, but wanted to hear from Chris or Steve, since they have a lot more experience with python. |
aeeefd2 to
91f3c3d
Compare
2ac8e62 to
b4af343
Compare
|
|
Todo:
|



This closes #119.
Pydicom warning:
get_frame_offsets is deprecated and will be removed in v4.0See instructions at https://github.com/pydicom/pydicom/blob/main/doc/release_notes/v3.0.0.rst that pydicom
get_frame_offsetsusage should be replaced withparse_basic_offsets.This PR maintains compatibility with pydicom >=2.2 which is supported per the pyproject.toml:
dicomweb-client/pyproject.toml
Line 36 in 84663e1
I discovered this deprecation usage while using pydicom 3 and latest dicomweb-client in the context of 3D Slicer as part of updating python packages to latest (Slicer/Slicer#8844).
cc: @fedorov